Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #225 #226

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Fix issue #225 #226

merged 4 commits into from
Dec 14, 2023

Conversation

lohedges
Copy link
Contributor

This PR closes #225, by fixing the custom_parameters keyword argument for AMBER protein force fields. This option was added when decoupling from the additional leap_commands option, which now allows the user to reference the loaded molecule. Importantly, custom_parameters must be placed at the start of the LEaP script, i.e. before the molecule is loaded. The option should be passed as a list of paths to custom parameter files, which will be loaded with loadAmberParams, or the names of additional internal leaprc files, e.g. leaprc.gaff, which will be sourced with source. I've added a test that validates that both types of file can be handled and that the output in the LEaP script is in the correct order.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@xiki-tempula

@lohedges lohedges added bug Something isn't working exscientia Related to work with Exscientia labels Dec 11, 2023
@lohedges lohedges temporarily deployed to biosimspace-build December 11, 2023 16:24 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build December 11, 2023 16:24 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build December 11, 2023 16:24 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build December 11, 2023 16:24 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build December 11, 2023 16:24 — with GitHub Actions Inactive
Copy link
Contributor

@xiki-tempula xiki-tempula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lohedges
Copy link
Contributor Author

I've now split this into pre_mol_commands and post_mol_commands, which is what I was originally thinking of doing. The pre_mol_commands can be used for adding things like custom parameter files, whereas post_mol_commands can be used for anything that needs to reference the molecule itself.

This changes the API, so I should really do this as a feature. The issue is that the current 2023.4 implementation is broken, so I do need to apply a fix there. Should I just revert the functionality there, i.e. move the position of the leap_commands? It would be a shame to need to apply this twice.

@xiki-tempula
Copy link
Contributor

Just revert to the old behaviour is good with me.

@lohedges
Copy link
Contributor Author

@chryswoods: Do you have any opinions on how to proceed? Should I merge this as a fix and backport to main as is, or merge as a feature and apply a separate fix to main. I don't think anyone is using this feature other than Exs and I assume that they are happy as long as it works. Given that the custom_parameters functionality is broken, I don't think anyone can be using it. (No reported issues.) If I were to apply a different fix on main then the easiest thing would be to remove the custom_parameters option entirely and revert the position at which the leap_commands are injected. (Annoying, my new solution is what I had originally proposed.)

@chryswoods
Copy link
Contributor

I think this can classify as a fix, so you can put it into the BioSimSpace patch release. It's at the borderline of being a feature, but if it isn't in widespread use, and we want a stable last patch while people play with 2023.5.0, then I think it is worth merging as a fix.

@lohedges lohedges merged commit b17b7c2 into devel Dec 14, 2023
5 checks passed
@lohedges lohedges deleted the fix_225 branch December 14, 2023 12:08
lohedges added a commit that referenced this pull request Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exscientia Related to work with Exscientia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] custom_parameters option not exposed via AMBER protein force field generator
3 participants